api for post upload #888

krmax44 · 2024-11-11T09:27:19Z

notable changes:

split up foirequest/api_views.py into multiple files. this is a breaking change i.e. for froide-campaign and froide-food. checkout their respective branches for testing:
- https://github.com/okfde/froide-food/tree/krmax44/foirequest-api
- https://github.com/okfde/froide-campaign/tree/krmax44/foirequest-api
add create/update methods to message endpoint
modified CreateOnlyWithScopePermission, so that regular authentication can also be used. please review carefully, see df7b5a3

ts client example:

import { retrieveFoiMessage } from './api/index.ts'

retrieveFoiMessage({
  path: { id: '22699' }
}).then((response) => {
  console.log(response.data?.kind) // "email" | "post" | "fax" | "upload" | "phone" | "visit" | "import" | undefined
})

…ot just tokens otherwise, we'd have to generate a token for our frontend

stefanw · 2024-11-19T09:45:55Z

froide/foirequest/admin.py

-        qs = super(FoiMessageAdmin, self).get_queryset(request)
-        qs = qs.prefetch_related("deliverystatus")
-        return qs
+        return self.model.objects.with_drafts(False).prefetch_related("deliverystatus")


The super().get_queryset(request) should be kept and extended with the filter, right? There's more happening in Django ModelAdmin's get_queryset, like ordering. .width_drafts(False) is fancy but only works on the manager and not the queryset which makes it difficult to compose.

stefanw · 2024-11-19T09:49:53Z

froide/foirequest/admin.py

+@admin.register(FoiMessageDraft)
+class FoiMessageDraftAdmin(FoiMessageAdmin):
+    def get_queryset(self, request):
+        return self.model.objects.all().prefetch_related("deliverystatus")


Should also call super and possibly just show drafts?

stefanw · 2024-11-19T09:51:23Z

froide/foirequest/api/permissions.py

+from ..auth import can_write_foirequest
+
+
+class OwnsFoiRequestPermission(permissions.BasePermission):


Maybe rename to WriteFoiRequestPermission? The concept of owning is not clearly defined in the code base apart from request.user == object.user which is not what is checked here.

stefanw · 2024-11-19T09:53:37Z

froide/foirequest/api/permissions.py

+        if request.method in permissions.SAFE_METHODS:
+            return True
+
+        return can_write_foirequest(getattr(obj, self.request_field), request)


I don't think this can work with belongs_to__request used in the FoiAttachmentViewSet as this is just attribute access.

stefanw · 2024-11-19T09:56:18Z

froide/foirequest/models/message.py

+# the other kinds can only be created by the system
+MESSAGE_KIND_USER_ALLOWED = [
+    MessageKind.POST,
+    MessageKind.PHONE,


We are not using PHONE or VISIT. Should we disallow their creation? Otherwise we might get random visit messages in the database.

stefanw · 2024-11-19T10:03:16Z

froide/foirequest/api/serializers.py

+            raise serializers.ValidationError("Only draft messages can be altered.")
+
+    def create(self, validated_data):
+        return super().create(validated_data)


I think we should create a FoiEvent for auditing purposes when the message is created and/or finalized, either directly or by sending a signal. (example)

stefanw · 2024-11-19T10:24:30Z

froide/foirequest/models/message.py

    def get_queryset(self):
-        return super().get_queryset().filter(is_draft=False)
+        return self.with_drafts()


Not calling super here is just a workaround because the method is only defined on the manager and doesn't exist on the queryset. Looks great as an API, but the problem I see here: we are using inheritance but then not calling super. A subtle bug may arise if FoiMessageManager get its own get_queryset method which is then not called.

Alternatives:

define a custom queryset instead which has the with_drafts method. You can create a matching manager by using objects = FoiMessageQueryset.as_manager().

Define a function with_drafts(qs: Queryset) -> Queryset and return with_drafts(super().get_queryset())

stefanw · 2024-11-19T10:34:32Z

froide/foirequest/models/message.py

@@ -201,6 +212,9 @@ def save(self, *args, **kwargs):

        super().save(*args, **kwargs)

+    def is_public(self) -> bool:
+        return self.is_draft is False


This works and is likely going to continue to work but it makes me uneasy as the intention here does not match the operator: comparing identity instead of checking truthiness. If you don't care about identity, don't compare it. It can be a subtle source of bugs because it can be an implementation detail. Favorite example:

>>> x = 3 >>> y = 3 >>> x is y True >>> x = 123123123123123 >>> y = 123123123123123 >>> x == y True >>> x is y False

What we want is return not self.is_draft.

stefanw · 2024-11-19T10:39:36Z

froide/foirequestfollower/api_views.py

                requests = [int(r) for r in requests]
-            except ValueError:
+            except (ValueError, AttributeError):


This should probably check self.request is None explicitly and return a value. Otherwise we might hide request.query_params no longer being an attribute or any other AttributeErrors. Keeping try/except small and focused is a good idea.

stefanw · 2024-11-19T10:42:00Z

froide/georegion/api_views.py

        try:
+            if self.request.user.is_superuser:
+                return GeoRegionDetailSerializer
            return self.serializer_action_classes[self.action]
        except (KeyError, AttributeError):


Same here, should handle self.request is None explicitly and bail with a value.

krmax44 force-pushed the krmax44/post-upload-api branch 5 times, most recently from a4aa599 to d7df699 Compare November 11, 2024 16:23

krmax44 force-pushed the krmax44/post-upload-api branch 3 times, most recently from 7e86c43 to c1424fe Compare November 18, 2024 14:58

krmax44 added 2 commits November 18, 2024 16:06

♻️ split up foirequest api views

7519879

🛂 allow creating/updating via api also with regular authentication, n…

1f42757

…ot just tokens otherwise, we'd have to generate a token for our frontend

krmax44 force-pushed the krmax44/post-upload-api branch 2 times, most recently from bd9e35e to d6e3dc4 Compare November 18, 2024 15:47

krmax44 added 3 commits November 18, 2024 17:02

✨ add create + update endpoints to message api, refactor api code

90d2692

✨ add generated typescript api client using openapi-ts

aba9dc5

✨ add FoiMessageDraft proxy model to remove drafts in admin

e9bbc04

krmax44 force-pushed the krmax44/post-upload-api branch from d6e3dc4 to e9bbc04 Compare November 18, 2024 16:02

krmax44 marked this pull request as ready for review November 18, 2024 16:03

krmax44 requested review from pajowu, stefanw and bikubi and removed request for pajowu November 18, 2024 16:03

stefanw requested changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api for post upload #888

api for post upload #888

krmax44 commented Nov 11, 2024 •

edited

Loading

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

stefanw Nov 19, 2024

		from ..auth import can_write_foirequest


		class OwnsFoiRequestPermission(permissions.BasePermission):

api for post upload #888

Are you sure you want to change the base?

api for post upload #888

Conversation

krmax44 commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmax44 commented Nov 11, 2024 •

edited

Loading